Skip to content

Generate sitemap for dynamic deploy url#4923

Closed
bhatia2akshit wants to merge 1 commit intoreflex-dev:mainfrom
bhatia2akshit:generate-sitemap-new
Closed

Generate sitemap for dynamic deploy url#4923
bhatia2akshit wants to merge 1 commit intoreflex-dev:mainfrom
bhatia2akshit:generate-sitemap-new

Conversation

@bhatia2akshit
Copy link
Copy Markdown

@bhatia2akshit bhatia2akshit commented Mar 7, 2025 •
edited
Loading

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?

After these steps, you're ready to open a pull request.

a. Give a descriptive title to your PR.

b. Describe your changes.

c. Put `closes #XXXX` in your comment to auto-close the issue that your PR fixes (if such).

masenf
masenf reviewed Mar 7, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty cool, a couple of suggestions,

  • the implementation should be outside of app.py and imported from there
  • consider generating the sitemap.xml during compile time and place the resulting file in the frontend assets to avoid having to generate xml for each request, when the content is unlikely to have changed.

@benedikt-bartscher
Copy link
Copy Markdown
Contributor

benedikt-bartscher commented Mar 7, 2025

  • consider generating the sitemap.xml during compile time and place the resulting file in the frontend assets to avoid having to generate xml for each request, when the content is unlikely to have changed.

There is a plan to introduce dynamic sitemaps, f.e. for a blog. This would require to implement 2 different approaches, one for static sitemap and one for dynamic sitemap. We could f.e. try to auto-detect if dynamic features are used or add a config option for that.

@benedikt-bartscher benedikt-bartscher mentioned this pull request Mar 7, 2025
Closed
9 tasks
@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 8, 2025 •
edited
Loading

@masenf running it before compile time would mean that backend must replace deploy url if its different from the ones in sitemap. I would think the better approach would be to run the sitemap generation when the app is deployed for the first time on the server. This would allow the same image to be deployed in different servers with unique sitemaps.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 11, 2025 •
edited
Loading

@masenf I have made changes as per your suggestions

  1. shifted sitemap generating methods into a separate file.
  2. added sitemap generation when the app is deployed on the server based on the deploy url. When a user request sitemaps, it checks if the sitemap exist in the static folder under .web directory. If not, new sitemaps are generated otherwise existing is served.
  3. add_page accept sitemap's priority (default is 10.0, signalling user didnt provide it and hence automatic needed) and changefreq (default is 'weekly')

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq bot commented Mar 11, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #4923 will improve performances by 3.44%

Comparing bhatia2akshit:generate-sitemap-new (7d7be04) with main (fae7b3c)

Summary

⚡ 1 improvements
✅ 11 untouched benchmarks

Benchmarks breakdown

Benchmark BASE HEAD Change
⚡ test_compile_stateful[_simple_page] 253.6 µs 245.2 µs +3.44%

masenf
masenf reviewed Mar 14, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this looks good, but when i reflex export, it looks like .web/_static directory is deleted. it's probably better to write into .web/public.

if possible, this should remove the next-sitemap dependency as we're not using it anymore.

run pre-commit and address unit test failures

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 14, 2025

hould remove the next-sitemap dependency as we're not using it anymore.

When you run reflex export, the _static folder is not created. The reason being i commented the next-sitemap generation. The deployment will create the _static folder.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 17, 2025

@masenf I have fixed the errors. Do you agree to my reasoning related to not needed to write to PUBLIC folder? Like, when we build our web app, the static folder is not created. when we start the app, sitemaps are generated and written in the static folder.

masenf
masenf reviewed Mar 17, 2025
View reviewed changes
Copy link
Copy Markdown
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work:

reflex run

http://localhost:8000/sitemap.xml -- no pages are listed

The following commands simulate a common production-like workflow and it also doesn't work with the sitemap logic as written:

reflex init
reflex export --frontend-only --no-zip
mv .web/_static /tmp/srv && (cd /tmp/srv && python3.12 -m http.server 3000) &
reflex run --backend-only
  1. http://localhost:3000/sitemap.xml is a 404
  2. http://localhost:8000/sitemap.xml does not list any pages

In a backend-only deployment, the page routes are not compiled, so app._pages is empty. That's why it doesn't really make sense to have get_pages. If you want to get all of the routes that would be compiled, you need to use app._unevaluated_pages.

I also still think it makes sense to export a static sitemap.xml with the frontend, so that it can be served even when the backend is down. If you also need to dynamically regenerate the sitemap.xml when the backend is running, i suppose that's okay too, but i still don't see the advantage of that personally.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 17, 2025

@masenf Yeahh I see the mistake in the approach. I will change the approach then.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 19, 2025

@masenf I have made the required changes, and tested it with reflex run. It works now.

@masenf masenf mentioned this pull request Mar 20, 2025
Merged
@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 22, 2025

@masenf @benedikt-bartscher, Does it fulfil all the requirements? or is there something else that needs to be added?

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Mar 31, 2025

@benedikt-bartscher @masenf will you or anyone else be able to help me with this error: https://github.com/reflex-dev/reflex/actions/runs/14047699692/job/39545884070?pr=4923? I am unable to reproduce it on my system. Also, pre-commit is passing in my system as well.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Apr 14, 2025

Thank you @adhami3310.

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Apr 24, 2025

Hi @masenf @adhami3310 is there something left that i must do? or can we merge into the master?

@adhami3310
Copy link
Copy Markdown
Member

adhami3310 commented Apr 24, 2025

sorry i got distracted with other things, if you resolve the merge conflicts we can get it for the next release

@bhatia2akshit
Copy link
Copy Markdown
Author

bhatia2akshit commented Apr 24, 2025

@adhami3310 done

@adhami3310
Copy link
Copy Markdown
Member

adhami3310 commented Jul 10, 2025

we implemented this in a very different way, we appreciate your help and apologize for unneeded delays

@adhami3310 adhami3310 closed this Jul 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@masenf masenf masenf left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@bhatia2akshit @benedikt-bartscher @adhami3310 @masenf